Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-120198: Stop the world when setting __class__ on free-threaded build #120672

Merged
merged 8 commits into from
Jul 10, 2024

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Jun 18, 2024

Include/object.h Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
}
err:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To have a more regular try: ... finally: _PyEval_StartTheWorld() pattern, you can add an int res = -1; variable, replace goto err with goto done, and set res to 0 on success (3 lines above).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @vstinner's suggestion is fine. Or you can refactor the parts that should be in a stop-the-world call into it's own function, like we often do for locks.

Another advantage of moving the body to a separate function is that it makes it more clear what data crosses the stop-the-world boundary -- some data loaded before the stop-the-world call may not be valid after it.

Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
@@ -323,6 +323,8 @@ _PyInlineValuesSize(PyTypeObject *tp)
int
_PyDict_DetachFromObject(PyDictObject *dict, PyObject *obj);

PyDictObject *_PyObject_materialize_managed_dict_lock_held(PyObject *);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's name this like the other functions: _PyObject_MaterializeManagedDict_LockHeld and move the definition up next to _PyObject_MaterializeManagedDict.

Objects/typeobject.c Outdated Show resolved Hide resolved
}
err:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @vstinner's suggestion is fine. Or you can refactor the parts that should be in a stop-the-world call into it's own function, like we often do for locks.

Another advantage of moving the body to a separate function is that it makes it more clear what data crosses the stop-the-world boundary -- some data loaded before the stop-the-world call may not be valid after it.

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few comments about comments.

I think this should be backported to 3.13

Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
@Fidget-Spinner Fidget-Spinner merged commit 3bfc9c8 into python:main Jul 10, 2024
34 checks passed
@miss-islington-app
Copy link

Thanks @Fidget-Spinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @Fidget-Spinner, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 3bfc9c831ad9a3dcf4457e842f1e612e93014a17 3.13

@Fidget-Spinner Fidget-Spinner deleted the class_stoptheworld branch July 10, 2024 18:02
Fidget-Spinner added a commit to Fidget-Spinner/cpython that referenced this pull request Jul 10, 2024
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants